Skip to content

🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing#2634

Open
tmshort wants to merge 1 commit intooperator-framework:mainfrom
tmshort:use-embed-to-update-profiles
Open

🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing#2634
tmshort wants to merge 1 commit intooperator-framework:mainfrom
tmshort:use-embed-to-update-profiles

Conversation

@tmshort
Copy link
Copy Markdown
Contributor

@tmshort tmshort commented Apr 8, 2026

Embed mozilla_data.json and parse it at init() time using the existing cipherSuiteId()/curveId() helpers. Unsupported ciphers are skipped and recorded in skippedCiphers, validated by TestNoSkippedCiphers. Simplify update-tls-profiles.sh to a curl download and drop the gojq dependency.

When the profile changes, make verify will show a diff of the profile. The build will still complete using the old data. The updated profile will need to be commited in order for it to be included in the build.

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Copilot AI review requested due to automatic review settings April 8, 2026 14:13
@openshift-ci openshift-ci bot requested review from anik120 and trgeiger April 8, 2026 14:13
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 8, 2026

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 970a111
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/69d903793539b30008e4edb2
😎 Deploy Preview https://deploy-preview-2634--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR replaces the generated mozilla_data.go file with an embedded JSON approach, simplifying the TLS profile management. Instead of generating Go code from the Mozilla SSL/TLS Configuration Guidelines, the PR embeds the JSON data and parses it at runtime during package initialization. This eliminates the dependency on gojq and makes profile updates clearer through simple JSON diffs.

Changes:

  • Embed mozilla_data.json using go:embed and parse at init() time instead of generating Go code
  • Simplify update-tls-profiles.sh to just download the JSON file, removing all jq-based code generation
  • Add TestNoSkippedCiphers to validate that all ciphers in the Mozilla data are supported by Go's crypto/tls
  • Remove gojq dependency from the Makefile

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/shared/util/tlsprofiles/mozilla_data.go Rewritten to embed and parse mozilla_data.json at runtime, with helper functions to convert Mozilla cipher/curve names to Go TLS identifiers
internal/shared/util/tlsprofiles/mozilla_data.json New embedded JSON file containing Mozilla TLS profile configurations for "modern" and "intermediate" profiles
hack/tools/update-tls-profiles.sh Simplified from 131 lines to 12 lines - now just downloads JSON from Mozilla instead of generating Go code
Makefile Removed gojq dependency from the update-tls-profiles target
internal/shared/util/tlsprofiles/tlsprofiles_test.go Added TestNoSkippedCiphers to ensure all ciphers in the embedded data are supported by Go

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.88%. Comparing base (b6dfd40) to head (970a111).

Files with missing lines Patch % Lines
internal/shared/util/tlsprofiles/mozilla_data.go 66.66% 8 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2634      +/-   ##
==========================================
- Coverage   68.91%   68.88%   -0.03%     
==========================================
  Files         139      140       +1     
  Lines        9863     9905      +42     
==========================================
+ Hits         6797     6823      +26     
- Misses       2559     2568       +9     
- Partials      507      514       +7     
Flag Coverage Δ
e2e 37.56% <66.66%> (-0.12%) ⬇️
experimental-e2e 52.34% <66.66%> (+0.04%) ⬆️
unit 53.59% <66.66%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tmshort tmshort changed the title 🌱 Replace generated mozilla_data.go with go:embed + runtime parsing 🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing Apr 9, 2026
Copy link
Copy Markdown
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2026
Copy link
Copy Markdown
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm


.PHONY: update-tls-profiles
update-tls-profiles: $(GOJQ) #EXHELP Update TLS profiles from the Mozilla wiki
env JQ=$(GOJQ) hack/tools/update-tls-profiles.sh
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me here we drop the only usage of gojq, but we keep it still in bingo files (.bingo/gojq.mod, .bingo/Variables.mk, ...).
Should we drop it also from bingo?
Should be something like:

bingo-v0.9.0 get gojq@none

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, if not used anywhere else, we should remove it from bingo files as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and rebased, please re-review @pedjak

@tmshort tmshort force-pushed the use-embed-to-update-profiles branch from d3e0d27 to 26ad8ee Compare April 10, 2026 13:38
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2026
Copy link
Copy Markdown
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2026
Copy link
Copy Markdown
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a great simplification — removes fragile shell code-gen and a dependency (gojq) in favor of go:embed + runtime parsing with existing helpers. A couple of inline suggestions below.

}
cipherNums = append(cipherNums, id)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsupported ciphers are tracked in skippedCiphers and validated by TestNoSkippedCiphers — nice. But unsupported curves (where curveId() returns 0) are silently dropped here with no tracking or test.

If Mozilla adds a new curve that Go does not yet support, this would silently weaken the profile. Consider adding a skippedCurves slice with a corresponding test, mirroring the cipher handling:

id := curveId(c)
if id == 0 {
    skipped = append(skipped, "curve:"+c)
    continue
}

(Or a separate skippedCurves list if you prefer to keep them distinct.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I noticed that too... it may not even be a curve that isn't support, but simply not mapped in our own functions.

func TestNoSkippedCiphers(t *testing.T) {
require.Empty(t, skippedCiphers,
"cipher(s) in mozilla_data.json are not supported by Go's crypto/tls and were omitted: %v", skippedCiphers)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test — catches newly-added ciphers that Go does not support yet.

Consider also adding a basic smoke test that asserts the parsed profile values match expectations (e.g. modernTLSProfile.minTLSVersion == tls.VersionTLS13, intermediate has ≥9 ciphers). This would catch upstream JSON schema changes that parse without error but produce wrong data. The existing tests exercise the profiles indirectly, so this is a nice-to-have rather than a blocker.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minTLSVersion could change out from under us... i.e. intermediate could go to a newer version; so I'd prefer to avoid explicit value checks. Even the number of ciphers could drop as they are determined to be weak, so checking for a minimum number of ciphers is probably not what we want.
The missing cipher/curve option is a much more appropriate check.

Embeds mozilla_data.json at compile time and parses it in init() to
populate modern and intermediate TLS profiles, removing the jq/gojq
dependency. Tracks skipped ciphers and curves (names not supported by
Go's crypto/tls) and asserts both are empty via TestNoSkippedCiphers
and TestNoSkippedCurves.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Todd Short <tshort@redhat.com>
Copilot AI review requested due to automatic review settings April 10, 2026 14:04
@tmshort tmshort force-pushed the use-embed-to-update-profiles branch from 26ad8ee to 970a111 Compare April 10, 2026 14:04
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2026
@pedjak
Copy link
Copy Markdown
Contributor

pedjak commented Apr 10, 2026

/approve

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 10, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak, rashmigottipati

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2026
Copy link
Copy Markdown
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants